Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid calling SoftReference.get() twice #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

turbanoff
Copy link
Contributor

SoftReference.get() is not supposed to be called more than once. It's because between two .get() calls GC could clear the reference and code could get an expected result.
In AspectJ it called twice in a few places, which we could easily update.

@kriegaex
Copy link
Contributor

kriegaex commented Apr 15, 2022

SoftReference.get() is not supposed to be called more than once.

Says who? Can you elaborate, please? I am not an expert on the subject matter, but I would rather think that get() should be called every time we want to use the referent, because precisely that could have happened: It was GC-ed in the meantime. The whole idea of a soft reference is to not use the referent anymore if it was GC-ed. By creating a strong reference and returning it, no matter what, we effectively block GC at the last second.

If your concern here is rather that the methods in question would return null in case GC happened before the second get() and that could lead to unexpected side effects, then I understand better. But that should not be a problem, because if get() yields null at the first call, we also return null. The only difference is that if the second call is null, we do not remove the map entry. But that would happen when it is requested the next time.

@kriegaex
Copy link
Contributor

@turbanoff, I know you seem to avoid writing a lot, which can be a nice quality sometimes. OTOH, you keep opening new PRs without responding to feedback in existing ones first. In order to keep the number of open PRs manageable, it would be nice to have a short cycle time for them. In the next few months I will have even less time than before due to my work to click through open issues. I also want to avoid letting your very welcome contributions rot. So please respond. Thank you so much.

@turbanoff
Copy link
Contributor Author

turbanoff commented Apr 21, 2022

By creating a strong reference and returning it, no matter what, we effectively block GC at the last second.

Actually, for JVM, even get() call creates a strong reference. It doesn't matter if code assigns it to variable or not. It was one of the reason, why new method Reference.refersTo was added in Java 16 (JDK-8188055).

I agree, that there are no real problems here in updated code. Let me update it to make it cleanup only in obvious cases (in remove)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants